Add backward compatibility for missing kind #214

zakiva · 2016-11-29T16:18:51Z

Adding static mapping for resources in order to keep compatibility for Openshift 3.1 (kubernetes v1.1.0-origin).

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1387614

simon3z · 2016-11-29T16:33:14Z

lib/kubeclient/common.rb

@@ -425,6 +425,7 @@ def api
    def load_entities
      @entities = {}
      fetch_entities['resources'].each do |resource|
+        resource = Kubeclient::Common::Compatibility31.compatible_resource(resource)


@zakiva shouldn't you do this only if resource['kind'].nil?

Yes, but I checked it inside the method: https://github.com/abonas/kubeclient/pull/214/files#diff-f1f5af126b7e6ff600fcc39decd74abfR64 thought it would be nicer to hide it.
Should I check it here instead?

@zakiva it's late here so don't assume that this works (or makes sense 😊)... but I would have expected something as:

fetch_entities['resources'].each do |resource| next if resource['name'].include?('/') resource['kind'] = Compatibility31.resource_kind(resource['name']) if resource['kind'].nil? ...

Instead of nil? you can also think to use empty? (I always forget the nits... anyway it would be nice to catch also when it's an empty string).

@moolitayer please check if the above makes sense.
For example, the check next if resource['name'].include?('/') is fully valid and working on 3.1 as well?

Yeah I'm good with that (.nil? since empty comes from rails) I'd definitely like to see a backward compatibility test.
@zakiva take a look at test/json/core_api_resource_list.json and it's use all over. You would probably want to rename it to core_api_resource_list_v13.json and add a core_api_resource_list_v11.json that you can test in a new file.

simon3z · 2016-11-29T16:33:47Z

@zakiva please link this on the BZ as well

zakiva · 2016-11-29T16:34:40Z

lib/kubeclient/compatibility_3_1.rb

+module Kubeclient
+  module Common
+    # Backward compatibility for 3.1
+    class Compatibility31


not sure regarding the naming here (and the file name)

In pure ruby it is not really important, in rails this would need to be named compatibility31.rb for you class name. I think we should change the class name to Compatibility11.

In the context of a "kubernetes client", 3.1 is unclear. OpenShiftEnterprise31 would be more explicit (is it OCP now? I think it was OSE when it was 3.1)

But I don't think we're 100% clear when it happened, and why downstream 3.1 was affected and origin 1.1 wasn't.
I tried to find when "kind" fields were added in openshift git log but gave up.
I compared upstream/downstream master config yaml — nothing jumped out either.
And I haven't tried kubernetes 1.1 (non-openshift) at all! (trying to compile it now, no luck so far)

Perhaps some neutral name like MissingKindCompatibility + a comment explaining we saw this happening in OSE 3.1?
That way if we learn more we won't need to rename the file/class.

@moolitayer @simon3z I would like to go with @cben's suggestion MissingKindCompatibility since we are not sure yet that kind is missing in k8s 1.1, are you OK with that?

moolitayer · 2016-11-29T17:16:08Z

@zakiva I asked to add a note in the readme about supported kubernetes versions.
Mention for 1.1 we support only core api not apis/extensions. Thanks!
(I think 1.1 is the correct k8s version, 3.1 is Openshift)

ps We need to make sure future manageiq code is aware of that, and uses other apis only if available.

cben

Nice brute force approach! 🎯 🔨

I assume you tested this on OSE 3.1 and things actually work? (e.g. ManageIQ refresh)

cben · 2016-11-30T08:50:57Z

lib/kubeclient/compatibility_3_1.rb

+      }
+
+      def self.compatible_resource(resource)
+        resource['kind'].nil? ? resource.merge!('kind' => MAPPING[resource['name']]) : resource


I'd prefer non-mutating merge over merge!. Or just:

resource['kind'] ||= MAPPING[resource['name']]

Or what simon3z suggested in calling function.

cben · 2016-11-30T12:21:02Z

lib/kubeclient/compatibility_3_1.rb

+module Kubeclient
+  module Common
+    # Backward compatibility for 3.1
+    class Compatibility31


In the context of a "kubernetes client", 3.1 is unclear. OpenShiftEnterprise31 would be more explicit (is it OCP now? I think it was OSE when it was 3.1)

But I don't think we're 100% clear when it happened, and why downstream 3.1 was affected and origin 1.1 wasn't.
I tried to find when "kind" fields were added in openshift git log but gave up.
I compared upstream/downstream master config yaml — nothing jumped out either.
And I haven't tried kubernetes 1.1 (non-openshift) at all! (trying to compile it now, no luck so far)

Perhaps some neutral name like MissingKindCompatibility + a comment explaining we saw this happening in OSE 3.1?
That way if we learn more we won't need to rename the file/class.

cben · 2016-11-30T13:04:50Z

README.md

@@ -198,6 +198,9 @@ You can also load your JSONified config in from an ENV variable (e.g. `KUBE_CONF
 ```
 Kubeclient::Config.new(JSON.parse(ENV['KUBE_CONFIG']), nil)
 ```
+###Supported kubernetes versions
+
+For 1.1 only the core api is supported, apis/extensions are supported in later versions.


We could in principle extend the mapping, right? It's just we don't yet need them?

Saying 1.1 is not precise, when I tried Origin 1.1 I did get kind fields. See my comment on naming.

I think that in order to support apis/extensions it will be more than just extend the mapping,
@moolitayer is it correct?

It should be in the mapping AFAIK. Since we have to manually test, review and add tests for everything we support I would rather we scope this change only for core api|oapi.

We would also need to check if we have cases of different apis using same entities.

@zakiva

we are supporting v1 only right? please state that here

can you figure out with @cben which is the currect k8s version to use here?

added 'v1'. @cben the point is that we know that for OSE 3.1 ( - kubernetes v1.1.0-origin ) kind is missing, so anyway we can't guarantee support for 1.1 and that's what I have mentioned in the readme.

zakiva · 2016-12-04T14:49:57Z

@moolitayer added tests, please take another look.

cben · 2016-12-04T14:56:09Z

LGTM

moolitayer · 2016-12-04T15:08:28Z

README.md

@@ -198,6 +198,9 @@ You can also load your JSONified config in from an ENV variable (e.g. `KUBE_CONF
 ```
 Kubeclient::Config.new(JSON.parse(ENV['KUBE_CONFIG']), nil)
 ```
+###Supported kubernetes versions
+
+For 1.1 only the core api v1 is supported, apis/extensions are supported in later versions.


apis/extensions is only one group api(the only one in 1.1 but not in later versions) please change to
For 1.1 only the core api v1 is supported, all api groups are supported in later versions.

moolitayer · 2016-12-04T15:12:13Z

test/test_resource_list_without_kind.rb

+      .to_return(body: open_test_file('core_api_resource_list_without_kind.json'),
+                 status: 200)
+
+    client = Kubeclient::Client.new 'http://localhost:8080/api/', 'v1'


parentheses please

moolitayer · 2016-12-04T15:14:16Z

test/test_resource_list_without_kind.rb

+      .to_return(body: open_test_file('core_oapi_resource_list_without_kind.json'),
+                 status: 200)
+
+    client = Kubeclient::Client.new 'http://localhost:8080/oapi/', 'v1'


parentheses please

moolitayer · 2016-12-04T15:38:56Z

LGTM 👍 cc @simon3z

simon3z · 2016-12-05T10:20:19Z

Nice! 👍

When the MissingKindCompatability class was introduced in ManageIQ#214, it didn't properly singularize SecurityContextConstraint as the kind in the MAPPING. When this gets loaded in the test/test_resource_list_without_kind.rb, it tries defining the `get_security_context_constraints` twice: * Once for the list action * Once for the record This means that you would not have both a singluar and plural method name for the entities.method_names list, and they both would be plural.

When the MissingKindCompatability class was introduced in ManageIQ#214, it the mapping has a plural form of the kind, instead of the singular form like most of the other entries. When this gets loaded in the test/test_resource_list_without_kind.rb, it tries defining the `get_security_context_constraints` twice: * Once for the list action * Once for the record This means that you would not have both a singluar and plural method name for the entities.method_names list, and they both would be plural. To fix, do something similar to what was done for the `Endpoints` entry, which is to have a rule for it in `Kubeclient::Common#parse_definition`

zakiva force-pushed the fix_refresh31 branch from 28d2d00 to ec492f3 Compare November 29, 2016 16:29

simon3z reviewed Nov 29, 2016

View reviewed changes

zakiva commented Nov 29, 2016

View reviewed changes

zakiva force-pushed the fix_refresh31 branch from ec492f3 to 2b8e077 Compare November 29, 2016 18:46

cben reviewed Nov 30, 2016

View reviewed changes

zakiva force-pushed the fix_refresh31 branch 7 times, most recently from 35612b9 to 9be2bc3 Compare December 4, 2016 14:36

moolitayer requested changes Dec 4, 2016

View reviewed changes

zakiva force-pushed the fix_refresh31 branch from 9be2bc3 to 393cea8 Compare December 4, 2016 15:24

moolitayer approved these changes Dec 4, 2016

View reviewed changes

zakiva mentioned this pull request Dec 4, 2016

Bump version to 2.3.0 #216

Merged

zakiva changed the title ~~Add backward compatibility for 3.1~~ Add backward compatibility for missing kind Dec 4, 2016

Add backward compatibility for missing kind

0dbcf00

zakiva force-pushed the fix_refresh31 branch from 393cea8 to 0dbcf00 Compare December 4, 2016 16:57

simon3z merged commit b41fc96 into ManageIQ:master Dec 5, 2016

NickLaMuro mentioned this pull request Aug 20, 2017

Fixes warnings in test output (and a potential bug) #261

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add backward compatibility for missing kind #214

Add backward compatibility for missing kind #214

zakiva commented Nov 29, 2016 •

edited

Loading

simon3z Nov 29, 2016

zakiva Nov 29, 2016

simon3z Nov 29, 2016

moolitayer Nov 30, 2016

simon3z commented Nov 29, 2016

zakiva Nov 29, 2016

moolitayer Nov 30, 2016 •

edited

Loading

cben Nov 30, 2016

zakiva Dec 4, 2016

moolitayer Dec 4, 2016

moolitayer commented Nov 29, 2016 •

edited

Loading

cben left a comment

cben Nov 30, 2016

cben Nov 30, 2016

cben Nov 30, 2016

zakiva Dec 4, 2016

moolitayer Dec 4, 2016

zakiva Dec 4, 2016

zakiva commented Dec 4, 2016

cben commented Dec 4, 2016

moolitayer Dec 4, 2016

moolitayer Dec 4, 2016

moolitayer Dec 4, 2016

moolitayer commented Dec 4, 2016

simon3z commented Dec 5, 2016

Add backward compatibility for missing kind #214

Add backward compatibility for missing kind #214

Conversation

zakiva commented Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon3z commented Nov 29, 2016

Choose a reason for hiding this comment

moolitayer Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Nov 29, 2016 • edited Loading

cben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zakiva commented Dec 4, 2016

cben commented Dec 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Dec 4, 2016

simon3z commented Dec 5, 2016

zakiva commented Nov 29, 2016 •

edited

Loading

moolitayer Nov 30, 2016 •

edited

Loading

moolitayer commented Nov 29, 2016 •

edited

Loading